Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Charge for size and witnesses #791

Merged
merged 63 commits into from
Jun 23, 2019
Merged

Charge for size and witnesses #791

merged 63 commits into from
Jun 23, 2019

Conversation

shargon
Copy link
Member

@shargon shargon commented May 29, 2019

Closes #840

@codecov-io
Copy link

codecov-io commented May 29, 2019

Codecov Report

Merging #791 into master will increase coverage by 4.83%.
The diff coverage is 61.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
+ Coverage   38.73%   43.57%   +4.83%     
==========================================
  Files         176      177       +1     
  Lines       12489    12547      +58     
==========================================
+ Hits         4838     5467     +629     
+ Misses       7651     7080     -571
Impacted Files Coverage Δ
neo/SmartContract/Native/NativeContract.cs 85.26% <ø> (ø) ⬆️
neo/SmartContract/Native/PolicyContract.cs 100% <ø> (ø) ⬆️
neo/SmartContract/WitnessWrapper.cs 0% <0%> (ø)
neo/Cryptography/Helper.cs 25.42% <0%> (+8.32%) ⬆️
neo/SmartContract/InteropService.cs 20.98% <100%> (+0.17%) ⬆️
neo/SmartContract/InteropService.NEO.cs 25.79% <14.28%> (+12.99%) ⬆️
neo/Network/P2P/Payloads/BlockBase.cs 69.11% <28.57%> (-7.94%) ⬇️
neo/SmartContract/Native/Tokens/GasToken.cs 38.23% <33.33%> (ø) ⬆️
neo/Network/P2P/Payloads/ConsensusPayload.cs 76.71% <33.33%> (-8.14%) ⬇️
neo/Consensus/ConsensusContext.cs 54.77% <50%> (ø) ⬆️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87ea7c6...5c1db4e. Read the comment docs.

@igormcoelho
Copy link
Contributor

I think I get the idea.. reuse network fee to cover system fee costs too, right?
Is it to avoid a possible "double spending" of the fee?
dont you.prefer the concept of witness/vscript consuming netfee, and script consuming sysfee?

@shargon
Copy link
Member Author

shargon commented May 30, 2019

Currently you don't pay for verification execution, with thi change you will pay

@erikzhang
Copy link
Member

Why not using NetworkFee for verification?

@shargon
Copy link
Member Author

shargon commented May 30, 2019

Why we don't unify both fees in only one?

@erikzhang
Copy link
Member

sys_fee is distributed to NEO holders. net_fee is distributed to consensus nodes.

@vncoelho
Copy link
Member

Do you think that fees to CN are important, Erik?
It wouls be nice to burn these fees, like Ripple...kkkkk

@erikzhang
Copy link
Member

Should we charge both the transaction size and the verification? Or should take the maximum of them?

@shargon
Copy link
Member Author

shargon commented May 30, 2019

I think that in verification we should pay for both, and in the invocation for the script execution. @vncoelho ripple could create more when they want 😂

@shargon
Copy link
Member Author

shargon commented May 30, 2019

But for me, only neo holders should receive the gas, CNs doesn't need it because, ussally, they are holders

@erikzhang
Copy link
Member

erikzhang commented May 31, 2019

So the algorithm is:

  1. Gas = InvocationCost
  2. NetworkFee = VerificationCost + tx.Length * FeePerByte

Is it correct?

Another way is:

  1. Gas = InvocationCost
  2. NetworkFee = max(VerificationCost, tx.Length * FeePerByte)

@shargon
Copy link
Member Author

shargon commented May 31, 2019

I preffer the first one

@shargon
Copy link
Member Author

shargon commented May 31, 2019

I think that we should have only one fee field, and CNs take his part from the same field.

@igormcoelho
Copy link
Contributor

igormcoelho commented May 31, 2019

@erikzhang @shargon I like a lot this direction. Could we consider Turing incomplete design for verification? So we dont even need to charge for length, only verification.

So I propose option 3: xD
Gas = invocation cost
NetworkFee = verification cost (no loops)

If not possible, I go with Erik option 1: netcost = verifcost + transmission cost

I think that we should have only one fee field, and CNs take his part from the same field.

Unified fee system is nice too. verification fee would go to CN, integer invocation fee to claims, and the rest burned?

@erikzhang erikzhang mentioned this pull request Jun 1, 2019
@erikzhang erikzhang added this to the NEO 3.0 milestone Jun 2, 2019
@erikzhang
Copy link
Member

We should remove low priority transactions from PolicyContract and MemoryPool.

@shargon
Copy link
Member Author

shargon commented Jun 2, 2019

I am worry about receiving Neo in my wallet and not being able to transfer it because i don't have enough gas. Other blockchains use the same token, so they could not have this problem, but we have two

@erikzhang
Copy link
Member

Don't worry. In NEO 3.0, we will distribute some gas in genesis block.

@shargon
Copy link
Member Author

shargon commented Jun 2, 2019

but if one user receive one neo, how much time he need to wait for generate enough gas for transfer it?

erikzhang
erikzhang previously approved these changes Jun 21, 2019
@shargon
Copy link
Member Author

shargon commented Jun 21, 2019

Done from my side

neo.UnitTests/UT_Transaction.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to make blockbase serialization more readable.

@shargon
Copy link
Member Author

shargon commented Jun 21, 2019

Trying to make blockbase serialization more readable.

In other PR please

@igormcoelho
Copy link
Contributor

But I'm not understanding it hahaha

Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, let's move on!

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Water flows, as well as us. aheuahuea
Let's move forward, continue discussions and evolve.

@igormcoelho
Copy link
Contributor

image

@shargon
Copy link
Member Author

shargon commented Jun 22, 2019

Merge?

@igormcoelho
Copy link
Contributor

Merge, merge, merge... 😂

@shargon shargon merged commit 90fa9af into neo-project:master Jun 23, 2019
@shargon shargon deleted the tx-fee branch June 23, 2019 00:22
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
* fix issue neo-project#791

* follow up

* follow up

* add example
Tommo-L pushed a commit to Tommo-L/neo that referenced this pull request Jun 22, 2020
* Fee= VerificationCost+ApplicationCost

* NetworkFee for verification

* Clean

* Clean

* Update

* Remove priority from Policy and MemoryPool

* Fix CalculateFee

* Allow to cancel transactions with more Gas

* Revert change

We need to test this first

* Add Cosigners

* some fixes

* Update Transaction.cs

* format

* fix overflow

* fix `Wallet.MakeTransaction()`

* fix `Transaction.CalculateFees()`

* remove contract get from verification

* Revert "remove contract get from verification"

This reverts commit 6d0dad2.

* Fix fee calculation

* fix

* fix tests

* Update MemoryPool.cs

* fix tests

* UT - Good calculation!

* Clean

* Revert Nep5Token.cs

* Revert conditional

* Improve readability

* Revert "Improve readability"

This reverts commit cd61b98.

* Possible fix for unit test

* Error verbosity

* Add using for ApplicationEngine

* Fix unit test

* more readable merkleroot test

* Sample for multisig contract

* Sign ut

* Signed!

* Same format for single signature unit test

* Prevent create unwanted objects

* format

* Remove `snapshot` from `MakeTransaction()`

* Rename

* format

* using json based NEP6Wallet import

* at least using read serializable array

* revert last commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NetworkFee calculation method
5 participants